Enum support for the argument parser #1196

smehringer · 2019-07-26T10:36:41Z

No description provided.

h-2 · 2019-07-29T12:03:13Z

In general we can do it this way. Some notes:

if it's a customisation point, it needs to be a CPO
I wouldn't use get_, because "get" usually means "access a member or element"
You can use a regular map instead of of an unordered_map to get the order. Performance is not relevant here as it's the arg parser and usually the lists are small.
The current implementation is entirely independent of enums. I think that's actually quite nice and we should document it as such.

smehringer · 2019-08-20T11:14:11Z

if it's a customisation point, it needs to be a CPO

But doesn't this work through ADL automatically?
Let's say the enum Foo of the user will be in the namespace Bar. Then the user will also define the function get_argument_conversion_table(Foo) in Bar and thus when I call the function within seqan3 code on the enum Bar::Foo then ADL will look whether Bar has this function implemented...
Where do I need a CPO and which class should be a CPO?

I wouldn't use get_, because "get" usually means "access a member or element"

Agreed. The name is ugly anyway but I am bad in naming. Suggestions? This also refers to naming it to something non-enum-specific.

h-2 · 2019-08-20T11:47:31Z

But doesn't this work through ADL automatically?

The same rules apply as anywhere else, or how is this different from to_char()?

Agreed. The name is ugly anyway but I am bad in naming. Suggestions? This also refers to naming it to something non-enum-specific.

They are name-value-pairs so I would name them something like that. I would probably make a name_value_table and implement it as a variable.

smehringer · 2019-08-20T12:21:55Z

The same rules apply as anywhere else, or how is this different from to_char()?

The difference is that we do not supply a global seqan3::get_argument_conversion_table.
Either there is this function in the bar namespace or not.

codecov · 2019-09-06T13:37:14Z

Codecov Report

Merging #1196 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1196      +/-   ##
==========================================
+ Coverage   97.52%   97.52%   +<.01%     
==========================================
  Files         226      226              
  Lines        8760     8778      +18     
==========================================
+ Hits         8543     8561      +18     
  Misses        217      217

Impacted Files	Coverage Δ
include/seqan3/argument_parser/argument_parser.hpp	`98.54% <ø> (ø)`	⬆️
include/seqan3/argument_parser/auxiliary.hpp	`100% <100%> (ø)`	⬆️
...ude/seqan3/argument_parser/detail/format_parse.hpp	`94.27% <100%> (+0.15%)`	⬆️
...lude/seqan3/argument_parser/detail/format_base.hpp	`89.31% <100%> (+0.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b234996...9bd13c3. Read the comment docs.

include/seqan3/argument_parser/argument_parser.hpp

test/snippet/argument_parser/string_convertible.cpp

test/unit/argument_parser/format_parse_test.cpp

test/unit/core/debug_stream_test.cpp

include/seqan3/argument_parser/auxiliary.hpp

include/seqan3/argument_parser/detail/format_parse.hpp

include/seqan3/core/detail/debug_stream_string_convertible.hpp

test/snippet/argument_parser/string_convertible.cpp

eseiler

One thing, otherwise LGTM

include/seqan3/argument_parser/detail/format_parse.hpp

h-2

The general structure and everything looks good, but some things should be changed I think:

if you call it a map, you should make it a variable and not a function. See for example alphabet_size for how that works with CPOs
"string conversion" is a very broad term, are you sure that's good? Maybe call it named_values? Name-value-pair and attribute-value-pair are popular names for this, I think... it also depends on the order of key/value... maybe also value_to_name_map.
enforce in the CPO that stuff actually has the right type
use ordered_map so that you have an order in the arg parser
make the name type a string_view instead of a string.

include/seqan3/argument_parser/auxiliary.hpp

include/seqan3/argument_parser/detail/format_base.hpp

include/seqan3/argument_parser/detail/format_parse.hpp

smehringer · 2019-10-09T12:11:34Z

@h-2 Can you take a look?
I am not sure if I got it completely right (although the tests pass..). I am unsure about when (or if) to remove cvref qualifiers?

h-2

As a a general design question: you seem to be added documentation to the man-page on which values are valid for the enum and you are using the value_list_validator for this. But then I don't see you actually using the value_list_validator as the validator!

And how does this relate to a (different) validator supplied by the user?

I would design it differently:

don't automatically assume anything about affected types.
provide an "enum_validator" that is a valid_values_validator with the extracted strings.

This adds less hacky-special-case code and allows people to e.g. select a subset of values that they want to accept.

include/seqan3/argument_parser/auxiliary.hpp

include/seqan3/argument_parser/detail/format_base.hpp

include/seqan3/argument_parser/detail/format_parse.hpp

include/seqan3/core/detail/debug_stream_named_enumeration.hpp

include/seqan3/argument_parser/detail/format_base.hpp

smehringer · 2019-10-29T14:26:49Z

I would design it differently:

don't automatically assume anything about affected types.

provide an "enum_validator" that is a valid_values_validator with the extracted strings.

This adds less hacky-special-case code and allows people to e.g. select a subset of values that they want to accept.

As discussed eprsonally I remove the automativ valid values but did not add a special enum_validator. The user can simply use map | views::get<1> for the value_list_validator as can be seen in the snippet example.

include/seqan3/argument_parser/auxiliary.hpp

test/snippet/argument_parser/custom_enumeration.cpp

test/unit/argument_parser/format_parse_test.cpp

include/seqan3/argument_parser/auxiliary.hpp

test/snippet/argument_parser/custom_enumeration.cpp

include/seqan3/argument_parser/detail/format_parse.hpp

h-2

This is all just minor stuff, feel free to merge this once these things have been fixed!

include/seqan3/argument_parser/auxiliary.hpp

include/seqan3/argument_parser/detail/format_parse.hpp

h-2 · 2019-11-12T11:42:09Z

include/seqan3/argument_parser/auxiliary.hpp

+ * It acts as a wrapper and looks for two possible implementations (in this order):
+ *
+ *   1. A static member `enumeration_names` in `seqan3::custom::argument_parsing<your_type>` that is of type
+ *      `std::unordered_map<std::string_view, your_type>>`.


I think we should relax these constraints to just be an "associative range", but that can be done later.

test/snippet/argument_parser/custom_argument_parsing_enumeration.cpp

h-2 · 2019-11-12T11:45:38Z

test/snippet/argument_parser/custom_argument_parsing_enumeration.cpp

+    // the static member function enumeration_names
+    // you can now add an option that takes a value of type std::errc:
+    parser.add_option(value, 'e', "errc", "Give me a std::errc value.", seqan3::option_spec::DEFAULT,
+                      seqan3::value_list_validator{(seqan3::enumeration_names<std::errc> | seqan3::views::get<1>)});


Is the mechanism of using seqan3::views::get documented anywhere?

What do you mean? This is a snippet that appears in the documentation of "how to specialise this". Is that documentation enough?

I just think that most users will be surprised that you can iterate over values/keys of an associative container using views::get<>.
If this is not explained anywhere, I think it would be good to add a sentence as explanation. But this can also be done later if you add a card.

test/unit/argument_parser/format_parse_test.cpp

h-2 · 2019-11-29T00:01:55Z

include/seqan3/argument_parser/auxiliary.hpp

+    {
+        return impl(priority_tag<1>{}, s_option_t{});
+    }
+};


This stuff could be much simpler, but it doesn't have to block this PR. We can fix it later.

h-2

I think these should work, too. If you do update this, please check. If not, it's not important.

Fell free to merge.

h-2 · 2019-11-29T00:03:06Z

test/snippet/argument_parser/custom_enumeration.cpp

+// Specialise a mapping from an identifying string to the respective value of your type bar.
+auto enumeration_names(bar)
+{
+    return std::unordered_map<std::string_view, bar>{{{"one", bar::one}, {"two", bar::two}, {"three", bar::three}}};;


Suggested change

return std::unordered_map<std::string_view, bar>{{{"one", bar::one}, {"two", bar::two}, {"three", bar::three}}};;

return std::unordered_map<std::string_view, bar>{{"one", bar::one}, {"two", bar::two}, {"three", bar::three}};;

?

h-2 · 2019-11-29T00:03:13Z

test/unit/argument_parser/detail/format_help_test.cpp

+
+auto enumeration_names(foo)
+{
+    return std::unordered_map<std::string_view, foo>{{{"one", foo::one}, {"two", foo::two}, {"three", foo::three}}};


Suggested change

return std::unordered_map<std::string_view, foo>{{{"one", foo::one}, {"two", foo::two}, {"three", foo::three}}};

return std::unordered_map<std::string_view, foo>{{"one", foo::one}, {"two", foo::two}, {"three", foo::three}};

h-2 · 2019-11-29T00:03:33Z

test/unit/argument_parser/format_parse_test.cpp

+{
+    static inline std::unordered_map<std::string_view, Other::bar> const enumeration_names
+    {
+        {{"one", Other::bar::one}, {"two", Other::bar::two}}


Suggested change

{{"one", Other::bar::one}, {"two", Other::bar::two}}

{"one", Other::bar::one}, {"two", Other::bar::two}

h-2 · 2019-11-29T00:03:49Z

test/unit/core/debug_stream_test.cpp

+
+auto enumeration_names(Foo)
+{
+    return std::unordered_map<std::string_view, Foo>{{{"one", Foo::one}, {"two", Foo::two}}};


Suggested change

return std::unordered_map<std::string_view, Foo>{{{"one", Foo::one}, {"two", Foo::two}}};

return std::unordered_map<std::string_view, Foo>{{"one", Foo::one}, {"two", Foo::two}};

smehringer · 2019-11-29T12:41:32Z

Fell free to merge.

Can you approve the PR then 😁 ?
I you don't and CI is through I'll merge it anyway

smehringer requested a review from h-2 July 26, 2019 10:37

h-2 removed their request for review August 2, 2019 12:02

smehringer force-pushed the argument_parser_fix branch from 8a3f4c8 to b535497 Compare September 6, 2019 13:37

smehringer force-pushed the argument_parser_fix branch 2 times, most recently from dbca685 to 6201278 Compare September 9, 2019 07:13

smehringer requested review from marehr and eseiler and removed request for marehr September 9, 2019 19:26

eseiler requested changes Sep 10, 2019

View reviewed changes

smehringer force-pushed the argument_parser_fix branch from dbbed0b to ddc4d81 Compare September 10, 2019 13:06

smehringer requested a review from eseiler September 10, 2019 13:07

eseiler requested changes Sep 10, 2019

View reviewed changes

smehringer force-pushed the argument_parser_fix branch 4 times, most recently from eda8ad3 to 1556454 Compare September 11, 2019 14:49

smehringer requested a review from eseiler September 13, 2019 14:59

eseiler approved these changes Sep 13, 2019

View reviewed changes

include/seqan3/argument_parser/detail/format_parse.hpp Outdated Show resolved Hide resolved

smehringer force-pushed the argument_parser_fix branch from 1556454 to b6349c7 Compare September 16, 2019 10:48

smehringer requested a review from h-2 September 16, 2019 10:49

h-2 reviewed Sep 16, 2019

View reviewed changes

smehringer force-pushed the argument_parser_fix branch 3 times, most recently from 2ca02ec to 67bbeb0 Compare October 9, 2019 12:10

smehringer requested a review from h-2 October 9, 2019 12:12

smehringer force-pushed the argument_parser_fix branch 4 times, most recently from 4c6fe75 to 7055baf Compare October 11, 2019 11:49

h-2 requested changes Oct 11, 2019

View reviewed changes

smehringer mentioned this pull request Oct 15, 2019

[MISC] Change value_list_validator construction. #1298

Merged

smehringer force-pushed the argument_parser_fix branch from 7055baf to 9521717 Compare October 29, 2019 14:24

smehringer requested a review from h-2 October 29, 2019 14:26

h-2 requested changes Oct 31, 2019

View reviewed changes

h-2 reviewed Oct 31, 2019

View reviewed changes

include/seqan3/argument_parser/detail/format_parse.hpp Outdated Show resolved Hide resolved

h-2 reviewed Oct 31, 2019

View reviewed changes

include/seqan3/argument_parser/detail/format_parse.hpp Outdated Show resolved Hide resolved

smehringer force-pushed the argument_parser_fix branch from 81052e7 to 840e08c Compare November 1, 2019 14:33

smehringer force-pushed the argument_parser_fix branch 2 times, most recently from d450b8a to 969fa64 Compare November 11, 2019 15:09

h-2 reviewed Nov 12, 2019

View reviewed changes

smehringer force-pushed the argument_parser_fix branch from 969fa64 to 3419310 Compare November 12, 2019 12:13

smehringer force-pushed the argument_parser_fix branch from 3419310 to 2870037 Compare November 21, 2019 16:09

smehringer requested a review from h-2 November 28, 2019 07:35

h-2 reviewed Nov 29, 2019

View reviewed changes

[FEATURE] Enum support for the argument parser.

9bd13c3

smehringer force-pushed the argument_parser_fix branch from 2870037 to 9bd13c3 Compare November 29, 2019 12:40

h-2 approved these changes Nov 29, 2019

View reviewed changes

h-2 merged commit 91211b1 into seqan:master Nov 29, 2019

eseiler mentioned this pull request Feb 16, 2021

[FIX] parsing containers of enums not working #2381

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enum support for the argument parser #1196

Enum support for the argument parser #1196

smehringer commented Jul 26, 2019 •

edited

Loading

h-2 commented Jul 29, 2019

smehringer commented Aug 20, 2019

h-2 commented Aug 20, 2019

smehringer commented Aug 20, 2019

codecov bot commented Sep 6, 2019 •

edited

Loading

eseiler left a comment

h-2 left a comment

smehringer commented Oct 9, 2019

h-2 left a comment

smehringer commented Oct 29, 2019

h-2 left a comment

h-2 Nov 12, 2019

h-2 Nov 12, 2019

smehringer Nov 21, 2019

h-2 Nov 28, 2019

h-2 Nov 29, 2019

h-2 left a comment

h-2 Nov 29, 2019

h-2 Nov 29, 2019

h-2 Nov 29, 2019

h-2 Nov 29, 2019

smehringer commented Nov 29, 2019

	return std::unordered_map<std::string_view, bar>{{{"one", bar::one}, {"two", bar::two}, {"three", bar::three}}};;
	return std::unordered_map<std::string_view, bar>{{"one", bar::one}, {"two", bar::two}, {"three", bar::three}};;

	{{"one", Other::bar::one}, {"two", Other::bar::two}}
	{"one", Other::bar::one}, {"two", Other::bar::two}

Enum support for the argument parser #1196

Enum support for the argument parser #1196

Conversation

smehringer commented Jul 26, 2019 • edited Loading

h-2 commented Jul 29, 2019

smehringer commented Aug 20, 2019

h-2 commented Aug 20, 2019

smehringer commented Aug 20, 2019

codecov bot commented Sep 6, 2019 • edited Loading

Codecov Report

eseiler left a comment

Choose a reason for hiding this comment

h-2 left a comment

Choose a reason for hiding this comment

smehringer commented Oct 9, 2019

h-2 left a comment

Choose a reason for hiding this comment

smehringer commented Oct 29, 2019

h-2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smehringer commented Nov 29, 2019

smehringer commented Jul 26, 2019 •

edited

Loading

codecov bot commented Sep 6, 2019 •

edited

Loading